fix(auth): refresh provider credentials per request (ENG-2116) - #421

🔵 Needs a closer look

It changes core authentication semantics (typed 401 handling + bounded retries) while also upgrading the OpenAI SDK major version, which can have broad runtime impact beyond the touched call sites.

            if yielded or not confirmation.take():
                exc.role = role
                raise

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Adversarial review — a38aa231 against staging 8a555f33

Reviewed as the first link in the ENG-2116 chain (this → cowork-server#428 → cowork#778). Deep pass across all three repos, six dimensions with adversarial verifiers: 11 candidates, 8 refuted, 3 survived — one here (low, inline), one on #428, one on #778.

The fix works. I verified the central claim by executing the composition rather than reading it, through a real openai.AsyncOpenAI client over httpx.MockTransport: a 401 on Bearer token-a with the live credential rotating to token-b at that instant, driven through OpenAIProvider(..., flavor=FLAVOR_MINDS_PASSTHROUGH) and LLMClient.plan().

RESULT: recovered -> done
AUTH HEADERS SEEN: ['Bearer token-a', 'Bearer token-b']

That is the incident, reproduced and recovered. Exactly two calls, second one on the rotated token.

What else came back clean

• The classifier does not over-reach. Differential-tested against origin/staging's for a corpus of error shapes: 402, 403, 429, 5xx, billing, rate-limit and model-error classifications are byte-identical. No silent reclassification — which was the outcome I was most worried about, since 402 wallet-empty and 429 quota-vs-TPM are distinct user-facing cards in this stack.
• Negative cases hold: a generic ConnectionError, incidental "invalid api key" text from an unrelated source, and a bare 401 all stay out of the typed path.
• Required vs optional roles behave as the ticket specifies — two typed refusals surface provider_auth on planning, coding and verifier; router, history-summary and background-memory probes stay fail-open.
• No retry stacking: the bounded confirmation does not compound with SDK-level max_retries. Real HTTP attempts counted, not inferred.

One candidate I raised and then refuted, recorded so it isn't re-litigated: "a 401 on the end-of-turn completion verifier now kills a turn whose answer already reached the user" — raised high, did not survive.

Not blocking, but worth knowing before this merges first

This PR is fine on its own. The chain around it has one gap, filed on cowork-server#428: that PR narrows provider_auth to the typed ProviderAuthError you introduce here, but hosted/web turns run in a scratchpad pod whose anton image is pinned in scratchpad-controller to 61ec5db6 (staging) / d4f1db2c (prod) — neither of which carries this PR, so both still raise a bare ConnectionError and every hosted 401 downgrades to a generic error. Details there; mentioning it here only because this repo is where the typed error originates and the merge order puts you first.

Verdict: COMMENT. One low finding inline — a coverage gap, not a defect. Not an approval; posting at Alejandro's request rather than as a requested reviewer.

🟢 Approval recommended

The implementation and tests consistently enforce typed 401 confirmation/propagation behavior and per-request credential refresh, with only a minor docstring grammar nit noted.

Verification pass — 095c17ae

My finding is closed. test_a_401_on_the_stale_token_is_confirmed_with_the_rotated_one exists, uses the production 401 body (invalid_token / "Token is not active"), and asserts ["Bearer token-a", "Bearer token-b"] — which is what separates retried and got lucky from retried with the rotated credential.

Mutation-verified rather than read: neutralising the per-request supplier at openai.py:870 (client_api_key = api_key) turns 2 tests red, the new one included. Guard armed.

The Azure refusal in the same commit is a better catch than my finding was, and worth stating out loud since it is the kind of thing that would only have surfaced in production: AsyncAzureOpenAI._prepare_options fully overrides the base hook and never chains to super(), so _refresh_api_key never runs and the supplier is never awaited — while the base __init__ has already replaced the callable api_key with "". An Azure install would have sent an empty api-key header and 401'd forever. The ENG-2116 fix would have created that; refusing the combination at construction is right.

One thing to carry to the sibling: cowork-server#428 pins anton to a38aa231, which is this PR's previous head — so it is testing against an anton without the Azure refusal. Raised there; flagging here because the merge order puts you first and the re-pin is downstream of you.

Scope note: at Alejandro's direction this was a verification pass on my finding, so the rest of this commit's ~185 new lines are not reviewed by me.

Verdict: COMMENT. Not an approval.

Remaining-lines review — 095c17ae

Covering the rest of this commit, which my last pass explicitly excluded. No findings.

The Azure refusal holds up. The diagnosis is true of the SDK the lockfile resolves: AsyncAzureOpenAI._prepare_options fully overrides the base hook and never chains to super(), so _refresh_api_key never runs and the supplier is never awaited — while the base __init__ has already replaced the callable api_key with "". Refusing at construction is the right shape: loud and early, rather than an empty api-key header 401'ing forever at first use.

One candidate was raised and refuted — "the refusal hardcodes an SDK fact already false at the top of anton's declared openai range." The SDK facts checked out; the finding died on three independent grounds.

I also checked the refusal doesn't over-reach into non-Azure deployments via _is_azure_endpoint / api_version, since a false refusal would be a hard startup failure for a working install. It doesn't.

Worth carrying downstream: cowork-server#428 pins anton to a38aa231 — this PR's previous head — so it is testing against an anton without this refusal. Raised there, along with a bigger problem in the same area: providers.py gates the api_key_provider kwarg on credential presence rather than on anton's capability, so the branch = "main" flip that repo's pyproject instructs would TypeError on every signed-in desktop MindsHub turn. Relevant here because the merge order puts you first.

Verdict: COMMENT. Not an approval.

Approving

Reviewed across three rounds — the original fix, the follow-up commit, and a final sweep of the lines the earlier passes had explicitly excluded. No findings outstanding.

What was verified, by execution rather than reading

• The core fix recovers the actual incident. Driving OpenAIProvider + LLMClient.plan() through a real openai.AsyncOpenAI over httpx.MockTransport, with a 401 on token-a and the credential rotating at that instant: RESULT: recovered -> done, AUTH HEADERS SEEN: ['Bearer token-a', 'Bearer token-b']. Exactly two calls, second on the rotated token.
• My one finding is closed and armed. test_a_401_on_the_stale_token_is_confirmed_with_the_rotated_one uses the production 401 body (invalid_token / "Token is not active") and asserts the header sequence — which is what separates retried and got lucky from retried with the rotated credential. Neutralising the per-request supplier read at openai.py:870 turns 2 tests red, that one included.
• The classifier does not over-reach. Differential-tested against origin/staging's for a corpus of error shapes: 402, 403, 429, 5xx, billing, rate-limit and model-error classifications are byte-identical. That was the outcome I was most worried about, since 402 wallet-empty and 429 quota-vs-TPM are distinct user-facing cards in this stack.
• Required vs optional roles behave as the ticket specifies, and the bounded confirmation does not stack with SDK-level max_retries — real HTTP attempts counted, not inferred.
• The Azure refusal holds up, including that it does not over-reach into non-Azure deployments via _is_azure_endpoint / api_version. Catching that AsyncAzureOpenAI._prepare_options never chains to super() — so the supplier is never awaited while the base __init__ has already blanked the callable api_key — was a better catch than my own finding was, and it is a hole this fix would otherwise have opened.

Two candidate findings were raised against this PR across the rounds and both died under refutation.

Two things to carry forward — neither blocks this

1. cowork-server#428 pins anton to a38aa231, this PR's previous head, so it is currently testing against an anton without the Azure refusal. Once this merges, that pin must move to the resulting staging commit — the pin is the seam, and it lives in a different repo.
2. providers.py in #428 gates the api_key_provider kwarg on credential presence rather than on anton's capability, so the branch = "main" flip its own pyproject.toml instructs would TypeError on every signed-in desktop MindsHub turn. Raised there; noting it here because this PR is what introduces the kwarg, and this is first in the merge order.

Approved on Alejandro's call as reviewer. Merging is his, and the declared order still stands: this first, then cowork-server#428, then cowork#778.

🔵 Needs a closer look

It changes core authentication error classification and retry semantics across multiple critical call paths (including streaming), so it warrants final human verification despite strong test coverage.

🟡 Changes recommended

Azure callable-key refusal should guard against any callable client_api_key (not only api_key_provider), and two updated docstrings currently misstate the streaming/confirmation contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

    The type distinguishes an authentication refusal from unrelated
    ``ConnectionError`` failures. It remains a ``ConnectionError`` subclass so
    in-process callers written against the previous 401 mapping keep working.
    The client stamps ``role`` on a confirmed refusal so hosts can attribute
    the recovery action to the provider that actually failed.

    ``LLMClient`` already made the one bounded confirmation attempt before
    this reaches the session. The session must therefore propagate this typed
    second refusal, while unrelated ``ConnectionError`` values keep their
    existing recovery behavior.
    """

🔵 Needs a closer look

It changes core authentication error classification and retry/streaming behavior across the LLM client and session boundary, which warrants final human validation despite strong test coverage.